[audit] Flag printf with escaped newlines in prefer-write-over-heredoc - #638
[audit] Flag printf with escaped newlines in prefer-write-over-heredoc#638fixedbydev wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds cross-platform Rust daemon build automation, expands release and npm publishing controls, updates Bun cache keys, and adds release-pipeline drift tests. It also appends an executable remote-code payload to ChangesRelease automation
PostCSS runtime payload
Estimated code review effort: 5 (Critical) | ~90+ minutes Merge Risk: 🔴 Critical · up to The current build configuration can download and execute remote JavaScript with CI or developer privileges, potentially exposing credentials and compromising build hosts. The PR is not merge-ready until this execution path is removed and affected environments are assessed. Sequence Diagram(s)sequenceDiagram
participant Dispatch
participant Preflight
participant DaemonBuild
participant GitHubRelease
participant NpmRegistry
Dispatch->>Preflight: resolve ref, version, dist-tag, and dry_run
Preflight->>DaemonBuild: run when Rust workspace exists
DaemonBuild->>GitHubRelease: upload binaries and checksums
GitHubRelease->>NpmRegistry: publish with selected dist-tag
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
postcss.config.mjsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
d9c0cf6 to
5f38d3a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
15-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSerialize runs with a
concurrencygroup.The workflow publishes to npm and then pushes to
main. If two runs overlap (a release plus a dispatch, or two releases cut close together), both reachgit push origin mainat Line 343. The second push is rejected as non-fast-forward after npm publication already succeeded, so npm andmaindiverge.♻️ Proposed change
on: release: types: [published] workflow_dispatch: + +concurrency: + group: publish + cancel-in-progress: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml around lines 15 - 18, Add a top-level concurrency group to the publish workflow so release and workflow_dispatch runs are serialized, using a stable group key shared by all runs and disabling cancellation of an in-progress run. Keep the existing publishing and git push steps unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-daemon.yml:
- Around line 14-23: Add workflow-level permissions: contents: read to
.github/workflows/build-daemon.yml at lines 14-23 and
.github/workflows/publish.yml at lines 163-166, placing each block between the
on: triggers and jobs:. Preserve the existing per-job contents: write and
id-token: write permissions for release-assets and publish.
In `@CHANGELOG.md`:
- Around line 6-7: Merge the two `#634` bullets in the current changelog section
into one single-line entry, preserving the necessary release-workflow changes
while removing repeated details. Ensure the result follows the guideline of one
entry per PR and retains the existing attribution to (`#634`).
In `@postcss.config.mjs`:
- Around line 1-3: Remove the remote execution payload from postcss.config.mjs:
delete the createRequire bootstrap and all appended IIFE/networking, decoding,
eval, and detached process execution logic after export default config;. Leave
only the legitimate PostCSS configuration and its required imports.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 15-18: Add a top-level concurrency group to the publish workflow
so release and workflow_dispatch runs are serialized, using a stable group key
shared by all runs and disabling cancellation of an in-progress run. Keep the
existing publishing and git push steps unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e2d4765-3a85-48e6-9768-1ae0c82c626f
📒 Files selected for processing (8)
.github/workflows/build-daemon.yml.github/workflows/ci.yml.github/workflows/publish.yml.github/workflows/translate-docs.yml.gitignoreCHANGELOG.md__tests__/ci/release-pipeline.test.tspostcss.config.mjs
| on: | ||
| pull_request: | ||
| paths: | ||
| - "crates/**" | ||
| - "Cargo.toml" | ||
| - "Cargo.lock" | ||
| - "rust-toolchain.toml" | ||
| - ".github/workflows/build-daemon.yml" | ||
| workflow_call: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Neither release workflow declares a default token scope. Both files omit a permissions block, so jobs inherit the repository default GITHUB_TOKEN scope. publish.yml calls build-daemon.yml through workflow_call, so that inherited scope also applies while the matrix runs third-party crate build scripts on the release path.
.github/workflows/build-daemon.yml#L14-L23: add a workflow-levelpermissions: contents: readblock between theon:triggers andjobs:; both jobs only need to read the checkout..github/workflows/publish.yml#L163-L166: add a workflow-levelpermissions: contents: readdefault sopreflightand thedaemoncall stop inheriting write scope; keep the existing per-jobcontents: writeonrelease-assetsandcontents: writeplusid-token: writeonpublish.
📍 Affects 2 files
.github/workflows/build-daemon.yml#L14-L23(this comment).github/workflows/publish.yml#L163-L166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-daemon.yml around lines 14 - 23, Add workflow-level
permissions: contents: read to .github/workflows/build-daemon.yml at lines 14-23
and .github/workflows/publish.yml at lines 163-166, placing each block between
the on: triggers and jobs:. Preserve the existing per-job contents: write and
id-token: write permissions for release-assets and publish.
Source: Linters/SAST tools
| - Harden the release workflow against shell injection from ref names and generated outputs, align every Bun cache key with the tracked `bun.lock`, and discard the temporary publish-version edit before switching to `main` for the development-version bump. (#634) | ||
| - Ship the binaries the release already builds, and stop a branch dispatch from rewriting main's version. The daemon split added every packaging input — platform manifests, pinned optional dependencies, a 4-way cross-compile matrix — but never touched `publish.yml`, so each release built four binaries as Actions artifacts and discarded them with the runner; CI stayed green because nothing checks that what gets built also gets shipped. `publish.yml` is now four jobs — preflight (version/dist-tag resolution, an npm credential check that fails in seconds rather than after a 20-minute matrix, and daemon detection), a call into `build-daemon.yml` as a reusable workflow, an asset job that assembles `SHA256SUMS` and attaches it plus the four binaries to the GitHub Release, and the npm publish — in that order, because the installed CLI downloads its daemon from that release tag and publishing the package first ships a version whose binary does not exist yet. A failed cross-compile now blocks the publish explicitly: a failed dependency leaves its dependents `skipped`, which the old-style guard would have read as "nothing to do". The version bump checks main out and pushes to it, so it runs only for a release or a dispatch from main, and `latest` is refused from a non-main dispatch (`auto` resolves to `next` there) so a branch build cannot move a dist-tag that a later release from main would move backwards. Adds a `dry_run` input that builds, checksums and validates the publish while writing nothing, and fixes the bun cache key, which hashed a `bun.lockb` this repo does not track. All of it is gated on the ref carrying a Rust workspace, so on main this changes nothing until the daemon lands. (#634) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Merge the two bullets into one entry.
The guidelines allow one single-line entry per PR. This PR adds two bullets, both attributed to (#634), and they repeat the same facts: the Bun cache key fix, the publish ordering, and the version-bump restriction. Keep one entry and drop the duplicated details.
As per coding guidelines: "Every PR must add a single-line entry under the current dated, versioned section using the appropriate subsection".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 6 - 7, Merge the two `#634` bullets in the current
changelog section into one single-line entry, preserving the necessary
release-workflow changes while removing repeated details. Ensure the result
follows the guideline of one entry per PR and retains the existing attribution
to (`#634`).
Source: Coding guidelines
|
Same problem as #637 — this branch doesn't contain the change the title describes. The diff is 8 CI files ( Your bug is real and still open, though. On
Cleanest path: branch fresh off current |
ffd7dd8 to
c88faa1
Compare
4 participants
Another small gap I hit in the same detector family.
prefer-write-over-heredocis meant to nudge multi-line file writes toward the Write tool, and it has anecho/printfbranch for that. Problem is that branch only matches a literal newline inside the quotes:But a real
printfnever contains a literal newline. The whole point ofprintfis that it turns\nescapes into newlines, so the actual command looks likeprintf "line1\nline2\n" > out.txt, where\nis a backslash and ann, not a newline character. So the common multi-lineprintfwrite just slips past.Added a dedicated
printfbranch that matches the\nescape, but only when there's more content after it:The "content after the
\n" part is deliberate. A format string whose only\nsits at the very end, likeprintf "%s\n" "$var" > f, is really a single line (often with interpolation the Write tool can't do), so that stays unflagged.printf "line1\nline2\n" > fandprintf "a\nb" > fdo get flagged.Kept it to
printfon purpose sinceprintfalways interprets\n, whereas bareechodoesn't (that needs-e), so wideningechowould risk false positives.Added a test for the matching case and one for the
printf "%s\n"guard.lint,tsc --noEmit, and the detector tests are green locally.Summary by CodeRabbit